-
Couldn't load subscription status.
- Fork 380
Fix FOUC by moving stylesheets to head #686
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughReworks both layouts to build main body content into a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Browser
participant Rails as Rails Controller
participant Layout as Layout (application / stimulus)
participant View as View Template
Browser->>Rails: GET page
Rails->>Layout: Render layout
Layout->>Layout: Render <head>:\n- append_stylesheet_pack_tag / append_javascript_pack_tag (Stimulus)\n- stylesheet_pack_tag (media: all, data-turbolinks-track)\n- javascript_pack_tag (defer, data-turbolinks-track)
Layout->>View: Render view/body\n(inside View: content_for :body_content may be defined)
View-->>Layout: content_for :body_content returned
Layout->>Browser: Send HTML with assembled head and `<%= yield :body_content %>` in body
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚀 Quick Review App Commands
Welcome! Here are the commands you can use in this PR:
/deploy-review-app
Deploy your PR branch for testing
/delete-review-app
Remove the review app when done
/help
Show detailed instructions, environment setup, and configuration options.
@coderabbitai
coderabbitai
bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/views/layouts/application.html.erb(1 hunks)config/initializers/react_on_rails.rb(1 hunks)
🔇 Additional comments (2)
config/initializers/react_on_rails.rb (1)
7-7: Configuration change looks good.Disabling
auto_load_bundlecorrectly prevents automatic stylesheet appending during component rendering, which enables movingstylesheet_pack_tagto the head. Ensure all required packs are now manually specified in the layout files.app/views/layouts/application.html.erb (1)
24-24: Specify pack name in javascript_pack_tag.Similar to line 9, this
javascript_pack_tagcall lacks an explicit pack name. It should likely specify which pack to load:<%= javascript_pack_tag('application', 'data-turbolinks-track': true, defer: true) %>Verify which JavaScript pack should be loaded here and whether the current syntax correctly loads the intended assets.
PR Review: Fix FOUC by moving stylesheets to head
Summary
This PR addresses a Flash of Unstyled Content (FOUC) issue by relocating stylesheet loading from the end of the <body> to the <head> section and disabling automatic bundle loading. The changes are minimal, focused, and well-documented.
Code Quality & Best Practices ✅
Strengths:
- Minimal, focused changes: Only 2 files modified with 2 additions and 2 deletions - exactly what's needed
- Excellent PR documentation: Clear problem statement, root cause analysis, and solution explanation
- Proper ordering: Stylesheet placement in
app/views/layouts/application.html.erb:9follows best practice of loading styles before content - Logical flow:
append_stylesheet_pack_tagis called beforestylesheet_pack_tag, which is the correct order
Observations:
- The change from
auto_load_bundle = truetofalseshifts responsibility from automatic to manual bundle management - This is appropriate given the need for precise control over stylesheet loading order
Potential Issues & Considerations
1. Missing pack argument in stylesheet_pack_tag ⚠️
Location: app/views/layouts/application.html.erb:9
The current code:
<%= stylesheet_pack_tag(media: 'all', 'data-turbolinks-track': true) %>
According to Rails/Shakapacker conventions, stylesheet_pack_tag typically requires a pack name as the first argument. The code appears to be missing which pack(s) to load. This might work if there's a default pack configured, but it's unclear and could be a bug.
Recommendation: Explicitly specify the pack name(s):
<%= stylesheet_pack_tag('application', media: 'all', 'data-turbolinks-track': true) %>
Or if loading multiple packs, verify which packs should be included here.
2. Same issue with javascript_pack_tag ⚠️
Location: app/views/layouts/application.html.erb:24
Similar to above, the JavaScript pack tag also lacks an explicit pack name:
<%= javascript_pack_tag('data-turbolinks-track': true, defer: true) %>
Note: These issues existed before this PR, but worth addressing.
3. Auto-load implications
With auto_load_bundle = false, you must manually specify all required packs. The PR description mentions "stimulus-bundle already specified," which appears correct in the layout. However, if additional components require other bundles, those would need manual specification now.
Recommendation: Add a comment in the config explaining this trade-off:
# Set to false to prevent automatic bundle appending during react_component rendering # This allows precise control over asset load order (styles in head, not body) # Note: All required packs must be manually specified in layouts config.auto_load_bundle = false
Performance Considerations ✅
Positive impacts:
- Eliminates FOUC: Moving stylesheets to
<head>ensures styles are available before first paint - Better Core Web Vitals: Reduces Cumulative Layout Shift (CLS) by preventing style-related reflows
- Optimal for SSR: With server-side rendering enabled (
config.prerender = true), having styles in head is critical
Concerns:
- Potential render blocking: Stylesheets in
<head>are render-blocking by default. However, this is the correct trade-off for preventing FOUC - No async/defer on stylesheets: While you can't defer CSS, consider if critical CSS inlining would further improve performance (future enhancement, not required for this PR)
Overall: The performance trade-offs are appropriate and well-considered.
Security Concerns ✅
No security issues identified:
- Changes are limited to asset loading order
- No user input handling or data processing modifications
- No changes to authentication, authorization, or data access patterns
- Turbolinks tracking attributes maintained for cache busting
Test Coverage ⚠️
Current state:
- No tests found that specifically verify stylesheet loading order
- No tests for FOUC prevention
- Existing system specs in
spec/system/test functionality but not asset loading
Impact assessment:
- The changes are relatively low-risk from a functionality perspective
- SSR continues to work (components render normally)
- Visual regression is the primary concern
Recommendations:
-
Manual testing checklist (as noted in PR description):
- ✅ Visit https://staging.reactrails.com/ with cache disabled
- ✅ Verify no FOUC on initial page load
- ✅ Test all major pages (comments, router demos, etc.)
- ✅ Verify SSR still renders correctly
- ✅ Check browser DevTools Network tab for proper load order
-
Consider adding a system spec (optional but valuable):
# spec/system/asset_loading_spec.rb require "rails_helper" describe "Asset Loading" do it "loads stylesheets in head before body content" do visit root_path # Get the HTML source html = page.html # Verify stylesheet_pack_tag appears before body content stylesheet_position = html.index('stylesheet_pack_tag') || html.index('<link') body_position = html.index('<body') expect(stylesheet_position).to be < body_position end end
- Visual regression testing (if available):
- Screenshot comparison could catch FOUC issues automatically
- Tools like Percy, Chromatic, or BackstopJS could help
Additional Observations
Documentation
- PR description is exemplary - clear, detailed, includes test plan
- Commit message follows good conventions with problem/solution structure
- Co-authored tag with Claude is transparent
Potential Future Enhancements
- Consider using
<link rel="preload">for critical stylesheets - Evaluate if critical CSS inlining would provide additional performance benefits
- Monitor Real User Monitoring (RUM) data to confirm FOUC elimination in production
Recommendation: Approve with Minor Suggestions ✅
This is a solid, well-reasoned change that addresses a real UX issue. The code quality is good, and the approach is sound.
Before merging:
⚠️ Verify thestylesheet_pack_tagandjavascript_pack_tagcalls work without explicit pack names - test in development/staging- ✅ Complete the manual test plan on staging
- ✅ Ensure all CI checks pass (run the full test suite)
Optional improvements:
- Add explicit pack names to
*_pack_taghelpers for clarity - Add a comment in the initializer explaining the
auto_load_bundle = falsetrade-off - Consider adding a basic system spec for asset load order
Great work on identifying and fixing this issue! The thorough documentation makes the change easy to review and understand.
Reviewed by: Claude Code
Review Date: 2025年10月16日
Code Review - PR #686: Fix FOUC by moving stylesheets to head
Summary
This PR addresses the Flash of Unstyled Content (FOUC) issue by relocating stylesheet loading from the end of <body> to the <head> section. The approach is sound and solves a real UX problem.
Code Quality & Best Practices
✅ Strengths:
- Well-documented PR description - Clear problem statement, root cause analysis, and solution explanation
- Consistent implementation - Both layout files (
application.html.erbandstimulus_layout.html.erb) are updated identically - Proper ERB syntax -
content_forblocks are correctly structured - Preserves existing functionality - All pack tags are maintained, just reordered
⚠️ Issues Identified:
CRITICAL: Missing configuration change
- The PR description states that
auto_load_bundle = falseshould be set inconfig/initializers/react_on_rails.rb, but this file is not included in the PR changes - Currently, line 7 of
config/initializers/react_on_rails.rbstill hasconfig.auto_load_bundle = true - Impact: Without this change, the solution may not work as described. The
react_componenthelper might still auto-append stylesheets, defeating the purpose of this PR - Recommendation: Add the configuration file change to this PR
Potential ordering issue:
<% content_for :head do %> <%= append_stylesheet_pack_tag('stimulus-bundle') %> <%= append_javascript_pack_tag('stimulus-bundle') %> <%= append_javascript_pack_tag('stores-registration') %> <% end %>
- This
content_forblock is defined inside the<body>tag but yields in the<head> - While Rails will collect and render this correctly, it's unconventional and may confuse developers
- Better approach: Define
content_for :headblocks before they're yielded, or move append calls directly into the<head>section
Potential Bugs & Issues
-
Execution order concern:
<%= yield :head %>in the<head>section will execute before thecontent_for :headblock in the body- Rails handles this by buffering content_for blocks, but the first request may not have the expected content
- Test this carefully with fresh page loads (not cached)
-
Missing from PR but mentioned in description:
- The configuration change to
auto_load_bundle = falseis critical - Without it, automatic bundle loading may still occur
- The configuration change to
-
Duplicate pack loading risk:
- With
auto_load_bundle = true(current state), components might still try to auto-load packs - Combined with manual pack specification, this could lead to duplicate asset loading
- Verify that assets aren't being loaded twice in dev tools Network tab
- With
Performance Considerations
✅ Improvements:
- Eliminates FOUC - Styles load before content renders, improving perceived performance
- JavaScript deferred -
defer: truemaintains non-blocking behavior - Proper asset ordering - CSS in head, JS deferred is optimal
⚠️ Potential concerns:
- Render-blocking CSS - Moving stylesheets to
<head>is correct but does block initial render (this is intentional and necessary) - No apparent optimization for critical CSS - Consider extracting critical CSS inline for even faster first paint (future enhancement)
Security Concerns
✅ No security issues identified:
csrf_meta_tagsproperly included- No user input being rendered
- Asset integrity maintained with
data-turbolinks-track: true
Test Coverage
-
No automated tests for FOUC fix:
- No new specs added to verify stylesheet loading order
- Consider adding a system/integration test that checks:
- Stylesheets are in
<head>tag - Stylesheets load before body content
- No duplicate asset loading
- Stylesheets are in
-
Manual testing only:
- Test plan relies entirely on manual verification
- System specs exist (
spec/system/pages_spec.rb, etc.) but none verify asset loading order
-
Suggested test additions:
# spec/system/asset_loading_spec.rb require 'rails_helper' RSpec.describe 'Asset Loading', type: :system do it 'loads stylesheets in head section' do visit root_path head_html = page.find('head', visible: false).native.inner_html expect(head_html).to include('stylesheet_pack_tag') end it 'does not load stylesheets in body' do visit root_path body_html = page.find('body').native.inner_html expect(body_html).not_to include('stylesheet_pack_tag') end end
Action Items
Before merging:
-
⚠️ CRITICAL: Add the missing configuration change:# config/initializers/react_on_rails.rb config.auto_load_bundle = false
-
Consider refactoring the
content_for :headpattern:- Move append pack tags directly into
<head>section, OR - Add comments explaining the unconventional pattern
- Move append pack tags directly into
-
Add automated tests to verify:
- Stylesheets render in
<head> - No FOUC occurs (check computed styles on page load)
- No duplicate asset loading
- Stylesheets render in
-
Verify in staging that:
- SSR still works correctly
- All pages load with styles immediately
- DevTools Network tab shows no duplicate requests
- All existing tests pass
Overall Assessment
Good work on identifying and solving the FOUC issue! The approach is sound, but the PR is incomplete without the auto_load_bundle = false configuration change mentioned in the description. Please add that file to the PR and consider the testing recommendations above.
Rating: Approve with changes requested
Review generated with 🤖 Claude Code
Problem: - Stylesheets were loading at bottom of body, causing FOUC with SSR - Moving stylesheet_pack_tag to head failed because react_component with auto_load_bundle=true would call append_stylesheet_pack_tag during rendering, which must come before stylesheet_pack_tag Solution: - Set auto_load_bundle = false in react_on_rails config - Manually specify required packs in layout (stimulus-bundle) - Move stylesheet_pack_tag to head after append calls - This ensures styles load before content renders, preventing FOUC 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Problem: - Stylesheets at bottom of body caused FOUC with SSR - Moving to head failed because react_component (with auto_load_bundle) calls append_*_pack_tag during render, which must execute before the main *_pack_tag helpers Solution: - Use content_for :head block in body for explicit append calls - yield :head in head section before calling main pack tags - This ensures all appends (explicit + auto from react_component) execute before main pack tags, allowing styles in head - Keeps auto_load_bundle enabled for automatic component packs Changes: - Wrap append_*_pack_tag calls in content_for :head blocks - Add yield :head in head section before pack tags - Move stylesheet_pack_tag and javascript_pack_tag to head - Applied to both application.html.erb and stimulus_layout.html.erb Result: Stylesheets load in head, eliminating FOUC while maintaining auto_load_bundle functionality. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
0dce1d0 to
5f7c02b
Compare
...ack tags Solution: - Wrap body content in content_for :body_content at TOP of layout - This executes all react_component calls first, triggering auto-appends - Then render head with append calls followed by main pack tags - Finally yield body content Execution order: 1. content_for :body_content executes (all react_component auto-appends happen) 2. Explicit append_*_pack_tag calls in head 3. stylesheet_pack_tag and javascript_pack_tag in head (with all appends registered) 4. yield :body_content outputs the body Result: Stylesheets load in head, eliminating FOUC, while preserving auto_load_bundle functionality. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
PR Review: Fix FOUC by moving stylesheets to head
Overview
This PR addresses Flash of Unstyled Content (FOUC) by restructuring the layout templates to load stylesheets in the <head> section before body content renders. The approach uses Rails' content_for pattern to manage execution order while maintaining auto_load_bundle = true.
Code Quality & Best Practices
✅ Strengths
-
Well-documented PR description: The PR provides excellent context explaining the problem, root cause, and solution approach. This makes the change easy to understand and review.
-
Correct use of Rails patterns: The
content_for :body_content/yield :body_contentpattern is a standard Rails approach for managing content blocks and is appropriate here. -
Maintains functionality: The change preserves existing functionality (SSR, auto-loading) while fixing the visual issue.
-
Consistent application: Both layout files (
application.html.erbandstimulus_layout.html.erb) receive identical treatment, ensuring consistency.
🔍 Observations & Suggestions
-
Naming convention: The PR description mentions using
content_for :head, but the actual implementation usescontent_for :body_content. While this works correctly, there's a slight documentation mismatch. The actual implementation is more semantically accurate since it's wrapping body content, not head content. -
Indentation and formatting: The changes maintain consistent ERB formatting and proper indentation. Good adherence to style conventions.
-
Asset tag ordering: The pack tags are now correctly positioned:
append_*_pack_tagcalls in head (lines 8-10)- Main
stylesheet_pack_tagandjavascript_pack_tagafter appends (lines 12-13) - This ensures all appends register before main tags render
Potential Issues
⚠️ Minor Concerns
-
Missing
yield :headin diff: The PR description mentions callingyield :headin the head section, but the diff doesn't show this. Looking at the implementation, it appears the approach evolved to usecontent_for :body_contentinstead, which is actually cleaner. However, this creates confusion between the description and implementation. -
Redux store hydration data location: In both layouts,
redux_store_hydration_datais rendered at the end of body. This is correct for hydration, but worth noting that it's now inside thecontent_for :body_contentblock, which shouldn't affect functionality but changes the rendering order slightly. -
No visual regression tests: The test plan mentions manual verification on staging, but there are no automated tests to prevent FOUC regressions. Consider adding:
- Visual regression tests (e.g., using Percy, Chromatic)
- Performance tests checking DOMContentLoaded timing
- Tests verifying stylesheet loads before body content
Performance Considerations
✅ Positive Impacts
-
Eliminates FOUC: Primary performance improvement from a user experience perspective - styles load immediately, eliminating visual jank.
-
Maintains defer on JavaScript: The
defer: trueattribute on JavaScript pack tags ensures scripts don't block rendering, which is optimal. -
Proper CSS positioning: Stylesheets in
<head>allow the browser to:- Start downloading CSS immediately
- Parse styles before rendering body
- Avoid repaints from late-loading styles
⚠️ Considerations
-
Potential for duplicate style processing: Since the layout now has a more complex execution order (body content evaluated first to populate
content_for, then head renders, then body renders), ensure there's no performance regression. Testing on production-like data would be valuable. -
JavaScript defer attribute: While
defer: trueis good for performance, ensure all components properly handle deferred script execution and don't assume scripts run before body content is parsed.
Security Concerns
✅ No New Security Issues
-
CSRF protection maintained:
csrf_meta_tagsremains in the head section. -
No new user input: Changes only restructure existing template logic without introducing new attack surfaces.
-
No changes to authentication/authorization: Asset loading changes don't affect security boundaries.
-
Turbolinks tracking maintained: The
data-turbolinks-trackattribute is preserved on both stylesheet and JavaScript pack tags.
Test Coverage
⚠️ Gaps Identified
-
No automated tests in PR: The PR doesn't include any test changes or additions.
-
Manual test plan only: While the test plan in the description is good, it relies entirely on manual verification:
- [ ] Visit https://staging.reactrails.com/ and verify no FOUC - [ ] Check all pages load with styles immediately applied - [ ] Verify SSR still working properly - [ ] Confirm all tests pass - [ ] Check Network tab for no duplicate asset loading -
Recommended test additions:
- Integration tests: Verify stylesheets appear in head in rendered HTML
- System tests: Test critical user journeys still work (navigation, forms, etc.)
- Performance tests: Measure First Contentful Paint (FCP) and compare before/after
- Visual regression tests: Capture screenshots to prevent FOUC regressions
Additional Recommendations
-
Update PR description: Align the "Solution" section with the actual implementation (using
:body_contentinstead of:head). -
Add comments in templates: Consider adding brief comments explaining why the
content_for :body_contentwrapper exists:<%# Wrap body content so layout can render head with all pack tags before body %> <% content_for :body_content do %> ... <% end %>
-
Monitor post-deployment:
- Check Real User Monitoring (RUM) metrics for FCP improvements
- Monitor for any SSR-related errors
- Verify no duplicate asset loading issues
-
Consider extracting shared layout code: Both
application.html.erbandstimulus_layout.html.erbare very similar. Consider:- A shared partial for navigation/footer
- A parent layout that both inherit from
- This would make future changes easier and reduce duplication
Summary
Overall Assessment: ✅ Approve with minor suggestions
This is a solid PR that solves a real UX problem (FOUC) using appropriate Rails patterns. The code changes are minimal, focused, and maintain backward compatibility. The main areas for improvement are:
- Test coverage (add automated tests)
- Documentation alignment (update PR description to match implementation)
- Consider adding explanatory comments in templates
The security and performance implications are positive, and the approach is sound. Once the manual test plan is completed successfully, this is ready to merge.
Code Quality: ⭐⭐⭐⭐ (4/5) - Clean, idiomatic Rails code with good structure
Test Coverage: ⭐⭐ (2/5) - Manual test plan only, no automated tests
Documentation: ⭐⭐⭐⭐ (4/5) - Excellent PR description, minor mismatch with implementation
Security: ⭐⭐⭐⭐⭐ (5/5) - No concerns
Performance: ⭐⭐⭐⭐⭐ (5/5) - Positive improvement
Code Review - PR #686: Fix FOUC by moving stylesheets to head
Overview
This PR addresses Flash of Unstyled Content (FOUC) by relocating stylesheet and JavaScript pack tags from the body to the <head> section. The implementation uses Rails content_for/yield pattern to manage execution order with React on Rails' auto-loading feature.
✅ Strengths
-
Correct Problem Identification: The PR correctly identifies and fixes the FOUC issue caused by CSS loading after SSR content renders.
-
Proper Execution Order: Using
content_for :body_contentto wrap body content and yielding it after pack tags is a clever solution that ensures:- All
append_*_pack_tagcalls (including auto-generated ones) execute first - Main pack tags render in the head with all dependencies registered
- Preserves
auto_load_bundle = truefunctionality
- All
-
Consistent Implementation: Both layout files follow the same pattern, maintaining consistency.
-
Performance Improvement: Moving CSS to
<head>follows web performance best practices and eliminates FOUC.
⚠️ Potential Issues
1. Execution Order Concern (app/views/layouts/application.html.erb:8-11)
The current implementation has append_*_pack_tag calls inside the <head> section but before the content_for :body_content block executes:
<head> <%= append_stylesheet_pack_tag('stimulus-bundle') %> <%= append_javascript_pack_tag('stimulus-bundle') %> <%= append_javascript_pack_tag('stores-registration') %> <%= stylesheet_pack_tag(media: 'all', 'data-turbolinks-track': true) %> <%= javascript_pack_tag('data-turbolinks-track': true, defer: true) %> </head> <body> <%= yield :body_content %> <!-- This is where react_component auto-appends happen --> </body>
Problem: The explicit append_*_pack_tag calls execute before the body content (which contains react_component calls with auto-appends). However, the main stylesheet_pack_tag and javascript_pack_tag also execute before the body, so they won't include the auto-appended packs from react_component helpers.
Expected behavior based on PR description:
"Wrap explicit
append_*_pack_tagcalls incontent_for :headblocks, then callyield :headbefore the main*_pack_taghelpers"
Suggested Fix:
<% content_for :body_content do %> <% content_for :head do %> <%= append_stylesheet_pack_tag('stimulus-bundle') %> <%= append_javascript_pack_tag('stimulus-bundle') %> <%= append_javascript_pack_tag('stores-registration') %> <% end %> <%= react_component "NavigationBarApp" %> <div class="container mx-auto px-4 flex-grow"> <%= yield %> </div> <%= react_component "Footer" %> <%= redux_store_hydration_data %> <% end %> <!DOCTYPE html> <html> <head> <title>React Webpack Rails Tutorial</title> <%= yield :head %> <!-- Explicit appends execute here --> <%= stylesheet_pack_tag(media: 'all', 'data-turbolinks-track': true) %> <%= javascript_pack_tag('data-turbolinks-track': true, defer: true) %> <%= csrf_meta_tags %> </head> <body class="min-h-screen flex flex-col bg-sky-50 text-gray-700"> <%= yield :body_content %> </body> </html>
This ensures:
- Explicit
append_*calls execute first (viayield :head) - Auto-appends from
react_componentexecute next (when:body_contentis defined) - Main pack tags execute last, including all appended packs
2. Missing DOCTYPE Declaration Order
The content_for :body_content block appears before the <!DOCTYPE html> declaration. While this works in ERB (since content_for doesn't output directly), it may confuse developers reading the code.
Recommendation: Add a comment explaining this pattern or restructure as suggested above.
🔍 Code Quality & Best Practices
Good Practices Observed:
- ✅ Using
defer: trueon JavaScript pack tags for non-blocking script loading - ✅ Including
data-turbolinks-track: truefor proper Turbolinks cache invalidation - ✅ Maintaining CSRF protection with
csrf_meta_tags - ✅ Comprehensive PR description with clear problem/solution
Minor Suggestions:
-
Indentation Consistency: The indentation looks correct, maintaining 2-space Ruby/Rails convention.
-
Comment Clarity: The existing comment about
redux_store_hydration_datais helpful. Consider adding a brief comment explaining thecontent_for :body_contentpattern for future maintainers.
🔒 Security Considerations
✅ No security concerns identified:
- CSRF protection remains intact
- No user input handling in these changes
- No exposure of sensitive data
- No changes to authentication/authorization
⚡ Performance Considerations
✅ Positive impact:
- Eliminates FOUC: Major UX improvement
- Proper CSS loading order: Stylesheets in
<head>prevent render-blocking at body level - Deferred JavaScript: Scripts load with
defer, maintaining non-blocking behavior
- If the execution order issue exists (see Issue Need to add Capybara Feature tests #1 ), some component-specific packs might not load correctly, potentially requiring additional HTTP requests or causing runtime errors.
🧪 Test Coverage
Current state: PR description includes manual test plan but no automated tests added.
Recommendations:
- Add a system/integration test to verify:
- Stylesheets appear in
<head>section - No FOUC occurs (can test by checking computed styles immediately after page load)
- All expected pack tags are present in the correct order
- Stylesheets appear in
Example test:
# spec/system/fouc_prevention_spec.rb require "rails_helper" RSpec.describe "FOUC Prevention", type: :system do it "loads stylesheets in the head section" do visit root_path # Verify stylesheet is in head expect(page).to have_css('head link[rel="stylesheet"]') # Verify scripts are deferred expect(page).to have_css('head script[defer]') end end
- Verify existing system tests still pass to ensure no regression
📋 Checklist for Author
Before merging, please verify:
- Test the execution order with
auto_load_bundle = true- verify component-specific packs load correctly - Run full test suite to ensure no regressions
- Test on staging environment (https://staging.reactrails.com/)
- Verify Network tab shows no duplicate asset loading
- Check that SSR still works correctly
- Confirm FOUC is eliminated across all pages
- Consider adding automated tests for this behavior
🎯 Recommendation
Conditional Approval
The approach is sound, but please verify the execution order concern (Issue #1 above). If the current implementation works correctly with auto_load_bundle = true, then it's good to merge after:
- Confirming all tests pass
- Testing on staging
- Adding a comment explaining the pattern
If issues arise with component pack loading, implement the suggested restructuring above.
💡 Additional Context
This is a common Rails pattern when dealing with helpers that need to register dependencies before the main tags render. The React on Rails gem's auto-loading feature adds complexity, making the execution order critical. Great job documenting the approach in the PR description!
Reviewed with Claude Code 🤖
Let me know if you'd like me to clarify any of these points or if you need help implementing the suggested changes!
/deploy-review-app
🎉 ✨ Deploy Complete! 🚀
🌐 ➡️ Open Review App
Deployment successful for PR #686, commit 4cc2aeb
🎮 Control Plane Console
📋 View Completed Action Build and Deploy Logs
✅ Review app for PR #686 was successfully deleted
Uh oh!
There was an error while loading. Please reload this page.
Summary
stylesheet_pack_tagto<head>sectioncontent_for :body_contentpattern to ensure proper execution orderauto_load_bundle = truefor automatic component pack loadingProblem
Stylesheets were loading at the end of the body (after SSR content), causing noticeable FOUC where navigation and content appeared unstyled initially, then styles flashed in after CSS loaded.
Root Cause
With
auto_load_bundle = true, React on Rails'react_componenthelper automatically callsappend_stylesheet_pack_tagduring rendering. Since Shakapacker requires append helpers to execute beforestylesheet_pack_tag, we couldn't simply move the stylesheet tag to the head - the body would render after the head, causing appends to happen too late.Solution
Render the body content FIRST using
content_for, then render the head with pack tags:Execution order:
content_for :body_contentexecutes first, rendering allreact_componentcalls and triggering their auto-appendsappend_*_pack_tagcalls in headstylesheet_pack_tagandjavascript_pack_tagrender with ALL appends registeredyield :body_contentoutputs the pre-rendered bodyThis ensures:
append_*_pack_tagcalls (explicit + auto from react_component) happen firstauto_load_bundlestays enabled for automatic component packsChanges
app/views/layouts/application.html.erb: Wrapped body incontent_for :body_content, moved pack tags to headapp/views/layouts/stimulus_layout.html.erb: Same pattern appliedconfig/initializers/react_on_rails.rb: No changes - keptauto_load_bundle = trueImpact
auto_load_bundlestill enabledRelated Issues
Test Plan
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com
This change is Reviewable